Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IndexPatternCreationConfigRegistry replaced by addIndexPatternType #39168

Merged
merged 4 commits into from
Jun 19, 2019
Merged

IndexPatternCreationConfigRegistry replaced by addIndexPatternType #39168

merged 4 commits into from
Jun 19, 2019

Conversation

streamich
Copy link
Contributor

@streamich streamich commented Jun 18, 2019 •
edited by lukeelmers
Loading

Summary

  • Replaces IndexPatternCreationConfigRegistry uiRegistry by array [] — @mattkime
  • Uses Map interface for all Embeddables registries — @stacey-gammon

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@streamich streamich requested a review from a team as a code owner June 18, 2019 13:41
@streamich streamich added the release_note:skip Skip the PR/issue when compiling release notes label Jun 18, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@mattkime
Copy link
Contributor

Overall, I think this looks good. Changing so many registries is a double edge sword - on one hand its nice to see something work across a number of registries but on the other when a test fails it can be harder to figure out whats at fault. I'm okay with using maps to replace the registries but I'd like to hear why a map was chosen if its not obvious from the code.

I've been creating throw away PRs to run tests on a subset of changes. Might be a useful technique when working through a PR like this.

@streamich
Copy link
Contributor Author

retest

@streamich
Copy link
Contributor Author

@mattkime Map is chosen because Embeddables registries are key-value stores actionRegistry.set(...) and actionRegistry.get(...).

@streamich
Copy link
Contributor Author

retest

@elastic elastic deleted a comment from elasticmachine Jun 19, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@mattkime
Copy link
Contributor

Looks great!

@streamich streamich merged commit 071a52d into elastic:master Jun 19, 2019
@streamich streamich deleted the registries-5 branch June 19, 2019 20:21
This was referenced Jun 19, 2019
Closed
Merged
streamich added a commit that referenced this pull request Jun 20, 2019
@streamich
[7.x] No common registry implementation (#39168) (#39309)
251142b 
* refactor: 💡 use new Map() in Embeddables

* refactor: 💡 remove IndexPatternCreationConfigRegistry

* refactor: 💡 make indexPatternTypes registry semantic

* fix: 🐛 fix TypeScript errors
@streamich streamich added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Jul 11, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@streamich streamich changed the title No common registry implementation IndexPatternCreationConfigRegistry replaced by addIndexPatternType Jul 12, 2019
@lukeelmers lukeelmers removed the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Jul 17, 2019
@lukeelmers lukeelmers mentioned this pull request Jul 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattkime mattkime mattkime approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.3.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@streamich @elasticmachine @mattkime @lukeelmers